feat(docker): Create non-root clp-user; Move USER and ENV directives after flattening (fixes #1379); Reduce layers using multi-line ENV (fixes #1378).
#1413
Conversation
…lp-package Dockerfile (fixes y-scope#1379); Reduce layers using multi-line ENV (fixes y-scope#1378).
WalkthroughMoved runtime ENV and USER metadata and package ownership into a final scratch stage in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Builder as Docker Builder
participant Base as "base stage (build)"
participant Final as "final stage (scratch)"
participant Image as "resulting image"
Builder->>Base: execute build steps → produce filesystem/artifacts
Note over Base: No runtime ENV/USER left here
Builder->>Final: FROM scratch\nCOPY --from=base / /
Final->>Final: ARG UID\ncreate clp-user (useradd) with UID\nset multi-line ENV (CLP_HOME, PATH, PYTHONPATH)\nset USER and WORKDIR\nCOPY --chown=${UID} clp-package → ${CLP_HOME}
Final->>Image: persist ENV and USER metadata in final image
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-13T03:32:19.293ZApplied to files:
📚 Learning: 2025-10-20T21:05:30.417ZApplied to files:
🪛 Checkov (3.2.334)tools/docker-images/clp-package/Dockerfile[low] 1-29: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) 🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
To fix #1410, we need to "Move USER and ENV directives after image flattening in clp-package Dockerfile"
| ENV CLP_HOME="/opt/clp" | ||
| ENV PATH="${CLP_HOME}/bin:${PATH}" \ | ||
| PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" | ||
| ENV PATH="${CLP_HOME}/sbin:${PATH}" |
There was a problem hiding this comment.
- We still have to keep three
ENVdirectives because the laterENVsettings depend on the former ones. - Technically, we could combine the
PATHsettings into a single line likePATH="${CLP_HOME}/sbin:${CLP_HOME}/bin:${PATH}". For readability, we previously agreed in feat(docker): Add container image containing the CLP package, for Docker Compose integration (resolves #1164). #1166 that we want to split them into separate lines. Although the aim is to cut down on number of layers, we still should keep the lines separated given the benefits of readability should outweigh the layer count reduction
| PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" | ||
| ENV PATH="${CLP_HOME}/sbin:${PATH}" | ||
|
|
||
| USER 1000:1000 |
There was a problem hiding this comment.
The messages of groups: cannot find name for group ID 1000 and I have no name! are a little scary. Could we find a way to make them disappear?
junhaoliao
changed the title
junhaoliao
changed the title
junhaoliao
changed the title
clp-user and move env setup after flattening (fixes #1378, fixes #1379).
junhaoliao
changed the title
clp-user and move env setup after flattening (fixes #1378, fixes #1379).clp-user and move env setup after flattening (fixes #1378, fixes #1379).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/docker-images/clp-package/Dockerfile (1)
18-21: Restore one-variable-per-ENV instructionWe previously agreed (PR #1166) to keep each
ENVdirective focused on a single variable for readability and to avoid order-dependent surprises. Please split the combinedENVback into separate statements forPATH,PYTHONPATH, andUSER, even if it costs an extra layer. Example:-ENV PATH="${CLP_HOME}/bin:${PATH}" -ENV PATH="${CLP_HOME}/sbin:${PATH}" \ - PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \ - USER="clp-user" +ENV PATH="${CLP_HOME}/bin:${PATH}" +ENV PATH="${CLP_HOME}/sbin:${PATH}" +ENV PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" +ENV USER="clp-user"Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/docker-images/clp-package/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
tools/docker-images/clp-package/Dockerfile
🪛 Checkov (3.2.334)
tools/docker-images/clp-package/Dockerfile
[low] 1-25: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
# Conflicts: # tools/docker-images/clp-package/Dockerfile
junhaoliao
requested review from
kirkrodrigues and
sitaowang1998
and removed request for
kirkrodrigues and
sitaowang1998
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/docker-images/clp-package/Dockerfile (1)
25-28: Confirm critical ownership issue and provide verified fix.The review comment accurately identifies a blocking issue. Verification confirms:
- Line 25 lacks
--chownflag: files copied to${CLP_HOME}remain owned by root (uid=0), blocking write access when the container runs asclp-user(uid=1000).- Line 27 lacks
--user-groupflag: useradd creates only a user without a corresponding group, causing group ID resolution warnings.The proposed diff is correct and follows Docker best practices. Apply these changes:
-COPY --link ./build/clp-package ${CLP_HOME} +COPY --link --chown=1000:1000 ./build/clp-package ${CLP_HOME} -RUN useradd --uid 1000 --shell /bin/bash --home-dir ${CLP_HOME} ${USER} +RUN useradd --uid 1000 --user-group --shell /bin/bash --home-dir ${CLP_HOME} ${USER}Note: An alternative (more explicit) approach would be to create the group first using
groupadd -g 1000 clp-userand then reference it withuseradd -u 1000 -g clp-user, but the proposed fix is a valid functional improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/docker-images/clp-package/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
tools/docker-images/clp-package/Dockerfile
🪛 Checkov (3.2.334)
tools/docker-images/clp-package/Dockerfile
[low] 1-29: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
| ENV PATH="${CLP_HOME}/sbin:${PATH}" \ | ||
| PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \ | ||
| USER="clp-user" |
There was a problem hiding this comment.
ENV consolidation contradicts prior agreement to keep lines split.
Lines 21-23 consolidate PATH and related settings into a multi-line ENV statement. However, based on learnings from PR #1414 and the prior agreement in PR #1166, ENV directives should remain split into separate lines for readability, especially since later ENV settings depend on earlier ones (e.g., PYTHONPATH and USER reference CLP_HOME). Please revert to separate ENV lines.
Apply this diff to split the ENV statements:
ENV CLP_HOME="/opt/clp"
ENV PATH="${CLP_HOME}/bin:${PATH}"
-ENV PATH="${CLP_HOME}/sbin:${PATH}" \
- PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \
- USER="clp-user"
+ENV PATH="${CLP_HOME}/sbin:${PATH}"
+ENV PYTHONPATH="${CLP_HOME}/lib/python3/site-packages"
+ENV USER="clp-user"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENV PATH="${CLP_HOME}/sbin:${PATH}" \ | |
| PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \ | |
| USER="clp-user" | |
| ENV PATH="${CLP_HOME}/sbin:${PATH}" | |
| ENV PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" | |
| ENV USER="clp-user" |
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/Dockerfile around lines 21 to 23, the ENV
entries were combined into a single multi-line ENV which contradicts the agreed
style and can break dependent expansions; revert to separate ENV directives so
each variable is set on its own line (e.g., one ENV for PATH, one for
PYTHONPATH, one for USER) ensuring later variables can safely reference CLP_HOME
and improving readability.
There was a problem hiding this comment.
we now only split if consolidation into the same command is not possible
There was a problem hiding this comment.
@junhaoliao, thank you for the clarification! I understand now that the policy has been updated: ENV directives should be consolidated into multi-line statements when possible, and only split when dependencies between variables require separate commands.
The current implementation is correct under this updated guidance:
- Line 19:
ENV CLP_HOME="/opt/clp"— separate because it's referenced by subsequent ENVs - Line 20:
ENV PATH="${CLP_HOME}/bin:${PATH}"— separate because PATH must be modified in sequence - Lines 21-23: Consolidated multi-line ENV for the second PATH modification, PYTHONPATH, and USER — correct since these can all be set together
I apologize for the confusion caused by my earlier comment based on the outdated policy from PR #1166. The consolidation here properly addresses issue #1378 (reducing layers) while maintaining correct dependency order.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1413
File: tools/docker-images/clp-package/Dockerfile:22-24
Timestamp: 2025-10-20T21:05:30.383Z
Learning: In the clp repository's Dockerfiles, ENV directives should be consolidated into multi-line ENV statements when possible to reduce image layers. ENV statements should only be split into separate commands when consolidation is not possible due to dependencies (e.g., when later variables must reference earlier ones that need to be set first, or when PATH must be modified sequentially).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
…p-package Dockerfile.
| # Flatten the image | ||
| FROM scratch | ||
| COPY --link --from=base / / | ||
| COPY --link --chown=${UID} ./build/clp-package ${CLP_HOME} |
There was a problem hiding this comment.
Although Docker's documentation states that --chown= supports usernames and performs lookups in /etc/passwd and /etc/group within the container to resolve UID/GID, we must use ${UID} instead of ${USER} here because such lookups are not yet supported when --link is used.
This limitation is a known issue in Docker Buildx, tracked at moby/buildkit#2987
junhaoliao
changed the title
clp-user and move env setup after flattening (fixes #1378, fixes #1379).clp-user; Move USER and ENV directives after flattening (fixes #1379); Reduce layers using multi-line ENV (fixes #1378).
|
|
||
| ARG UID=1000 | ||
| ENV CLP_HOME="/opt/clp" | ||
| ENV PATH="${CLP_HOME}/bin:${PATH}" |
There was a problem hiding this comment.
I think this PATH can be moved inside the multi line env with something like
PATH="${CLP_HOME}/sbin:${CLP_HOME}/bin:${PATH}"
There was a problem hiding this comment.
For readability, would it be better to keep them on separate lines?
There was a problem hiding this comment.
I think this is fine from readability side. Appending two elements to a list is not hard to understand.
| PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" \ | ||
| USER="clp-user" | ||
|
|
||
| RUN useradd --uid ${UID} --shell /bin/bash --home-dir ${CLP_HOME} ${USER} |
There was a problem hiding this comment.
the user home path setting here broke the s3 profile auth type because the CONTAINER_AWS_CONFIG_DIRECTORY's value should be relative to the CLP_HOME but isn't
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the S3 profile auth type breakage
gh issue create \
--repo y-scope/clp \
--title "S3 profile auth type broken due to user home path change in Docker image" \
--body "## Problem
The user home path setting introduced in PR #1413 broke the S3 profile auth type because \`CONTAINER_AWS_CONFIG_DIRECTORY\`'s value should be relative to \`CLP_HOME\` but isn't.
## Context
- PR #1413 sets the home directory for \`clp-user\` to \`CLP_HOME\` (\`/opt/clp\`) in the Docker image
- \`CONTAINER_AWS_CONFIG_DIRECTORY\` is currently hardcoded to \`/.aws\`
- This mismatch breaks S3 profile authentication
## Related
- PR: https://github.com/y-scope/clp/pull/1413
- Comment: https://github.com/y-scope/clp/pull/1413#discussion_r2476592326
- Reported by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #1524 to track the S3 profile auth type breakage caused by the user home path change. The issue documents how the CONTAINER_AWS_CONFIG_DIRECTORY hardcoded path conflicts with the new home directory setting.
🧠 Learnings used
Learnt from: junhaoliao
PR: y-scope/clp#1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Learnt from: junhaoliao
PR: y-scope/clp#1413
File: tools/docker-images/clp-package/Dockerfile:22-24
Timestamp: 2025-10-20T21:05:30.417Z
Learning: In the clp repository's Dockerfiles, ENV directives should be consolidated into multi-line ENV statements when possible to reduce image layers. ENV statements should only be split into separate commands when consolidation is not possible due to dependencies (e.g., when later variables must reference earlier ones that need to be set first, or when PATH must be modified sequentially).
Learnt from: junhaoliao
PR: y-scope/clp#1178
File: components/clp-package-utils/clp_package_utils/controller.py:217-223
Timestamp: 2025-09-25T05:13:13.298Z
Learning: The compression scheduler service in CLP runs with CLP_UID_GID (current user's UID:GID) rather than CLP_SERVICE_CONTAINER_UID_GID (999:999), unlike infrastructure services such as database, queue, redis, and results cache which run with the service container UID:GID.
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.436Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: junhaoliao
PR: y-scope/clp#1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
2 participants
Description
This PR:
USERandENVdirectives to after the image flattening step in theclp-packageDockerfile. This ensures that the environment variables and user settings are correctly applied to the final image, rather than being overridden or lost during the flattening process. (fixes Docker image flattening loses USER and ENV directives in clp-package Dockerfile #1379)clp-useruser with a home directory and shell in the final image, ensuring that the user environment is properly set up./opt/clpfor theclp-user, ensuring that the user starts in the correct location when running a container from this image.PATHincludes both/opt/clp/binand/opt/clp/sbin, allowing access to all installed binaries.ENVdirectives into a single multi-lineENVdirective. This helps to optimize the image size and improve build performance. (fixes Optimize ENV declarations in clp-package Dockerfile to reduce layers #1378)Checklist
breaking change.
Validation performed
before
without the changes in the PR
after
with the changes in the PR
Observed that:
[+] Building 1.1s (12/12)->Building 5.3s (14/14).uid=0(root) gid=0(root) groups=0(root)->uid=1000(clp-user) gid=1000(clp-user) groups=1000(clp-user).PATHwas set as expected:PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin->PATH=/opt/clp/sbin:/opt/clp/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin.LS_COLORSwas lost, which is even better.CLP_HOMEis set as expected: CLP_HOME missing ->CLP_HOME=/opt/clp.HOMEis set as expected:HOME=/root->HOME=/opt/clp.PYTHONPATHis set as expected:PYTHONPATHmissing ->PYTHONPATH=/opt/clp/lib/python3/site-packages.Summary by CodeRabbit